-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LTS CMake Option in GenHarmBase Executables #6057
Conversation
If we don't use or build EvolveGhCce or EvolveGhCcm can I delete them in this PR (saw a comment on this in slack)? |
7050d51
to
006e31d
Compare
Do you understand why the tests fail? Let's discuss if needed! |
7de9fcc
to
db54fd0
Compare
Okay, got all the tests timing out figured out and clang-tidy so I think this is ready for a review :) |
Check: parse;execute_check_output | ||
Timeout: 8 | ||
Timeout: 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need this long of a timeout now that you decreased the slab size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah no, after looking at how long it took the tests in github, it seems 15 seconds should be good
db54fd0
to
2d823db
Compare
Squashed the timout change in :) @knelli2 |
# This is the smallest interval we'd need to observe time step/constraints. If | ||
# you need it smaller you can edit it, but make sure to change the slab | ||
# intervals in the EventsAndTriggers | ||
InitialSlabSize: 0.001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't restrict the slab size to line up with observations, particularly not to something this small. If you need more frequent output use dense output instead of forcing extra RHS evaluations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was sort of arbitrarily chosen to make the test not take forever. This wouldn't be very realistic for an actual evolution. Maybe I should change the comment to make it clear that you should change the slab size if you're using this yaml or use dense output if you want more frequent output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, having the value small for the test is fine, but the comment shouldn't recommend doing observations that way.
support/Pipelines/Bbh/Ringdown.yaml
Outdated
# This is the smallest interval we'd need to observe time step/constraints. If | ||
# you need it smaller you can edit it, but make sure to change the slab | ||
# intervals in the EventsAndTriggers | ||
InitialSlabSize: 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the observations in this file are based on the slab size, so you've decreased the amount of output by a factor of 50 here. For the constraint norms that might be fine, but it's probably not what you want for the actual data observations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken to match the inspiral.yaml and I assumed we'd want a similar amount of output during the ringdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. The dynamical timescales are shorter, so maybe people want more data? @geoffrey4444 or someone who actually analyzes runs want to weigh in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I actually did in the ringdown that worked; I'd suggest something similar in the pipeline:
Evolution:
InitialTime: 0.0
InitialTimeStep: 0.0002
InitialSlabSize: 0.1
StepChoosers:
- Increase:
Factor: 2
- ErrorControl:
AbsoluteTolerance: 1e-10
RelativeTolerance: 1e-8
MaxFactor: 2
MinFactor: 0.25
SafetyFactor: 0.95
TimeStepper:
AdamsBashforth:
Order: 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any actual successful ringdown runs with global time stepping, so I don't have an opinion on good choices for that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do you do observations? That's more what I'm worried about here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we know exactly how often we want to do observations. 0.1
for a slab size seems reasonable for now. We'll probably have to revisit this later though as we do more testing and actually do a successful ringdown with the pipeline. I think the more important part of this PR is the switch to LTS itself.
bbe3fe9
to
b2781c1
Compare
I'm fine with the settings in the files, but I still don't want comments telling people to shrink the slab to observe more. |
@wthrowe So what is your suggestion if more frequent observation is needed? Dense triggers? |
Yes. |
b2781c1
to
41aa6a7
Compare
Okay, changed the comments in the yamls to suggest dense output, let me know if this works @wthrowe |
Yes, that's fine. |
Ignoring unrelated test timeouts/failures |
Proposed changes
Adds an option to either build in LTS or GTS for the executables that use GeneralizedHarmonicBase
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments